Add secure HTTP server communication transport - #73
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9159c1ee8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b92ea397b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1faa2d2159
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c01f2ccea2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80a55dada1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
The proxy endpoint handlers currently conflate malformed requests with rate limiting (429) and there are a couple of fail-closed file reads that should size-check before reading entire files to avoid avoidable memory pressure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a reusable HTTPS long-poll transport for SimpleAPI server communication with mutual TLS enrollment/renewal, durable replay/ack fencing, and strict wire-format parsing to preserve compatibility with existing VotingPlugin transport semantics.
Changes:
- Added mTLS identity management, enrollment authority, and pinned-TLS client/server contexts for secure backend↔proxy communication.
- Implemented a strict, versioned HTTP transport protocol with bounded message sizes, canonical parsing, and delivery/ack semantics.
- Added extensive runtime and security regression tests covering durability, recovery, bounded resources, and concurrency behaviors.
File summaries
| File | Description |
|---|---|
| SimpleAPI/src/test/java/com/bencodez/simpleapi/servercomm/http/HttpTransportSecurityTest.java | Security/durability regression tests for enrollment, pins, fencing, recovery, and bounded state. |
| SimpleAPI/src/test/java/com/bencodez/simpleapi/servercomm/http/HttpTransportRuntimeTest.java | End-to-end runtime and concurrency tests for long-poll delivery/ack behavior and durability. |
| SimpleAPI/src/main/java/com/bencodez/simpleapi/servercomm/http/HttpTransportSecrets.java | Small crypto helpers (random tokens, SHA-256, HMAC) used by the transport and codes. |
| SimpleAPI/src/main/java/com/bencodez/simpleapi/servercomm/http/HttpTransportProtocol.java | Defines strict wire protocol, canonical parsing, and size bounds for transport packets. |
| SimpleAPI/src/main/java/com/bencodez/simpleapi/servercomm/http/HttpTlsIdentity.java | Durable private CA + server leaf identity with rotation/renewal logic. |
| SimpleAPI/src/main/java/com/bencodez/simpleapi/servercomm/http/HttpProxyTransportServer.java | HTTPS proxy listener implementing enrollment, renewal, and transport endpoints plus durable queues. |
| SimpleAPI/src/main/java/com/bencodez/simpleapi/servercomm/http/HttpPinnedTls.java | Pinned trust and secure TLS parameter helpers for backend-side clients. |
| SimpleAPI/src/main/java/com/bencodez/simpleapi/servercomm/http/HttpInboundDeliveryStore.java | Durable replay fence around non-transactional callbacks. |
| SimpleAPI/src/main/java/com/bencodez/simpleapi/servercomm/http/HttpEnrollmentAuthority.java | Single-use enrollment and durable binding/revocation state management. |
| SimpleAPI/src/main/java/com/bencodez/simpleapi/servercomm/http/HttpConnectionCode.java | Copy/paste connection code encoding/decoding with corruption MAC and endpoint validation. |
| SimpleAPI/src/main/java/com/bencodez/simpleapi/servercomm/http/HttpClientCredentialStore.java | Owner-only persistence of enrolled client credentials + profile with generation roll-forward/rollback. |
| SimpleAPI/src/main/java/com/bencodez/simpleapi/servercomm/http/HttpBackendTransportConnector.java | Backend long-poll connector with bounded IO, renewal, ordering, and durable ack fencing. |
| SimpleAPI/src/main/java/com/bencodez/simpleapi/file/DurableFiles.java | Cross-platform fsync/rename durability helpers used by transport state. |
| SimpleAPI/pom.xml | Adds Bouncy Castle dependencies and pins surefire version for the new tests. |
Review details
Suppressed comments (2)
SimpleAPI/src/main/java/com/bencodez/simpleapi/servercomm/http/HttpProxyTransportServer.java:191
- This returns HTTP 429 when the request body is malformed (missing/invalid Content-Length or Transfer-Encoding present). 429 indicates rate limiting; clients should instead get a request error (e.g., 400/411/413), and 429 should be reserved for admission control.
if (!"POST".equals(exchange.getRequestMethod())) { reply(exchange, 405, new byte[0]); return; }
if (!json(exchange)) { reply(exchange, 415, new byte[0]); return; }
if (!boundedFixedBody(exchange, 8192) || !admission.tryAcquire()) { reply(exchange, 429, new byte[0]); return; }
SimpleAPI/src/main/java/com/bencodez/simpleapi/servercomm/http/HttpProxyTransportServer.java:205
- This returns HTTP 429 when the request body is malformed (missing/invalid Content-Length or Transfer-Encoding present). 429 indicates rate limiting; clients should instead get a request error (e.g., 400/411/413), and 429 should be reserved for admission control.
if (!"POST".equals(exchange.getRequestMethod())) { reply(exchange, 405, new byte[0]); return; }
if (!json(exchange)) { reply(exchange, 415, new byte[0]); return; }
if (!boundedFixedBody(exchange, HttpTransportProtocol.MAX_BODY_BYTES) || !admission.tryAcquire()) { reply(exchange, 429, new byte[0]); return; }
try {
- Files reviewed: 14/14 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea26fde6ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d8e9c8d11
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 989ec3f28b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🔵 Needs a closer look
It adds large, security-critical custom mTLS/enrollment, private-CA lifecycle, crash-durability, and concurrency logic that warrants final human review despite thorough tests.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
SimpleAPI/src/main/java/com/bencodez/simpleapi/servercomm/http/HttpEnrollmentAuthority.java:59
- This error message states the lifetime must be "between one second and fifteen minutes", but the guard only rejects null, negative, zero, or values greater than 15 minutes. Any positive sub-second duration (e.g. 1 ms) is accepted, so the stated one-second lower bound is not enforced. Either enforce a one-second minimum or reword the message to match the actual accepted range (positive and at most fifteen minutes). The suggested fix takes the behavior-preserving option of correcting the wording.
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: BenCodez <17074231+BenCodez@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6a202ce39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9b9e6f2e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a large, security-critical transport surface area (mTLS enrollment, durable state, lifecycle/concurrency) that warrants careful final human review despite strong test coverage.
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5e84743c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fe4343d95
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2754e9670
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a994f0923
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bf06f981e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 826e680754
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af984b80ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf61b6f2e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| serverId = HttpTlsIdentity.canonicalServerId(serverId); | ||
| if (lifetime == null || lifetime.compareTo(Duration.ofSeconds(1)) < 0 || lifetime.compareTo(MAX_ENROLLMENT_LIFETIME) > 0) | ||
| throw new IllegalArgumentException("Enrollment lifetime must be between one second and fifteen minutes"); | ||
| Instant expiresAt = clock.instant().plus(lifetime); |
There was a problem hiding this comment.
When the clock has a fractional second and the caller requests the accepted one-second minimum, this computes an expiry such as 12:00:01.999, but HttpConnectionCode.encode() stores only getEpochSecond(), so the parsed code expires at 12:00:01.000 and may have only 1 ms of usable life. A modest scheduling delay can therefore make a freshly issued minimum-lifetime code unusable. Fresh evidence after the prior fix is that the new lower-bound check accepts exactly one second without compensating for the existing seconds-only encoding.
Useful? React with 👍 / 👎.
| private final Path stateFile; | ||
| private final Map<String, Enrollment> enrollments = new HashMap<>(); | ||
| private final Map<String, ClientBinding> bindings = new HashMap<>(); | ||
| private final Map<String, Instant> renewalNotBefore = new HashMap<>(); |
There was a problem hiding this comment.
With multiple live HttpEnrollmentAuthority instances sharing the same state directory—the configuration exercised by HttpEnrollmentAuthorityOwnershipTest—each instance keeps an independent renewalNotBefore map, and that map is neither loaded nor persisted under the cross-process state lock. A backend can alternate renewal requests between proxy listeners and obtain a replacement from each within the advertised one-minute window, repeatedly triggering certificate generation and complete state rewrites. Fresh evidence after the earlier rate-limit fix is that durable authority operations now explicitly coordinate across instances while the limiter remains process-local.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2e5f7bfb4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private static final String DIRECTORY = "http-transport-inbound-deliveries"; | ||
| // This sidecar is deliberately outside the deletable journal directory. Never remove it: | ||
| // deleting and recreating a locked file could split ownership across two inodes. | ||
| private static final String OWNER_LOCK_PREFIX = ".http-inbound-owner-"; | ||
| private static final String OWNER_LOCK_SUFFIX = ".lock"; |
There was a problem hiding this comment.
When backend identities are churned, sealAndDeleteIfEmpty() removes each retired journal directory but releaseOwnership() deliberately leaves its .http-inbound-owner-<id>.lock behind permanently. discover() tolerates these files while the backend cap counts only active journal directories, so repeated enrollment/revocation or replacement of backend IDs accumulates one inode per historical ID without limit and can eventually exhaust the state filesystem despite MAX_BACKENDS; use a bounded locking scheme or a safe protocol for reclaiming sidecars.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f451f78944
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| releaseOwnershipForRetirement(); | ||
| DurableFiles.deleteIfExists(sidecar); |
There was a problem hiding this comment.
When deleting the ownership sidecar or forcing its parent directory fails, releaseOwnershipForRetirement() has already cleared and closed this store's lock, while sealAndDeleteIfEmpty() propagates the exception before marking the backend retired. The BackendState therefore remains in backends, but every later retirement or inbound write fails requireWritable(), so at the backend cap this entry can permanently prevent admitting a replacement until the proxy restarts. Fresh evidence after the earlier retirement fix is that the new sidecar-reclamation path introduces another fallible cleanup step after ownership is released; either make this failure terminal so the state is removed or restore usable ownership for retries.
Useful? React with 👍 / 👎.
Summary
Validation
mvn -B -f SimpleAPI/pom.xml packageSimpleAPI/target/SimpleAPI.jar(15,044,219 bytes)git diff --check origin/main...HEADNo findings.Integration order
VotingPlugin PR #1594 remains unchanged and will switch to this shared implementation only after this PR is merged and the resulting SimpleAPI artifact is available.